🚅 Deployed to the rivet-pr-5010 environment in rivet-frontend

Service	Status	Web	Updated (UTC)
frontend-cloud	❌ Build Failed (View Logs)	Web	May 13, 2026 at 8:20 pm
frontend-inspector	❌ Build Failed (View Logs)	Web	May 13, 2026 at 8:20 pm
kitchen-sink	❌ Build Failed (View Logs)	Web	May 13, 2026 at 8:18 pm
website	😴 Sleeping (View Logs)	Web	May 12, 2026 at 3:59 am
ladle	❌ Build Failed (View Logs)	Web	May 9, 2026 at 3:40 pm
mcp-hub	✅ Success (View Logs)	Web	May 9, 2026 at 3:40 pm

• [slop]fix(workflow-engine): only commit step state after success #5010 : 2 dependent PRs (#5050 , #5052 ) 👈 (View in Graphite)
• fix(rivetkit): bind methods through createWriteThroughProxy #4987
• fix(rivetkit): decode bridge errors with actor: null #5056
• fix: wasm-pack build #5048
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Code Review: fix(workflow-engine): only commit step state after success

Note: This PR is still in Draft status with an empty description/checklist.

Overview

This PR fixes a correctness bug in workflow-engine where actor state mutations inside a failing step were being persisted even though the step needs to be retried. On retry, the step would execute against incorrectly mutated state. The fix:

1. Introduces RAW_STATE_SYMBOL to bypass the write-through proxy and read raw actor state.
2. Wraps every step() and tryStep() call in #withActorAccessAndStateRollback, which snapshots state+vars before the step and rolls them back on any error.
3. Removes the per-branch flushStorage() calls in step failure catch blocks, consolidating to the single flush in setRetryState / executeWorkflow's unrecoverable-error path.
4. Removes the stepData.error fallback (justified by the new test).

The core idea is correct and addresses a real workflow replay bug.

Bug: Breaks stateless actors using workflow steps

withActorAccessAndStateRollback unconditionally calls this.#runCtx[RAW_STATE_SYMBOL]() (workflow/context.ts:618), but ActorContextHandleAdapter[RAW_STATE_SYMBOL] throws stateNotEnabledError() when the actor has no state configured (native.ts:2460-2465).

This means any actor that uses workflow steps but does not have a state field will crash on the snapshot call, breaking all ctx.step() invocations regardless of whether the step touches state. The guard needs to handle the state-disabled case, for example by returning undefined and skipping the rollback assignment when #stateEnabled is false.

Concern: structuredClone(vars) can throw for non-cloneable objects

vars are runtime-in-memory (not persisted), so they can legitimately hold class instances, Promises, or WeakRefs. structuredClone throws a DataCloneError on these, crashing the snapshot before the step runs. Worth documenting that vars must be structured-cloneable when workflows are used, or guarding the clone.

Minor: Notification fires before flush for timeout/critical errors

Previously flushStorage() was called before notifyStepError for StepTimeoutError/CriticalError. Now the notify fires before the eventual top-level flush in executeWorkflow. This slightly widens the crash window between notification and persistence (duplicate notification possible on replay). Not a durability regression, but worth a comment.

Code Quality

Positive changes:

• Consolidating entry.kind.data.error and entry.dirty = true before all the if-branches eliminates three identical blocks.
• Removing the double-flush on failure (catch block plus setRetryState) is correct; the top-level flush in executeWorkflow guarantees persistence on all error paths.
• Removing the stepData.error fallback is justified since the new test confirms the error is always written to the step entry.

Issues:

1. Misleading test name (steps.test.ts:573): 'should not commit step error data to entry on failure' but the test asserts expect(entry.kind.data.error).toBe('Error: step failed'), i.e. the error is committed. Rename to 'should commit error but not output to step entry on failure'.

2. Bare catch {} in the new test's first-run block silently swallows unexpected errors. Add a comment explaining what error is expected.

3. // @ts-nocheck in workflow/context.ts: the RAW_STATE_SYMBOL call is unverified by the type system. The interface declares the method returning TState, with TState = never for stateless actors the type system would surface the mismatch if checking were enabled.

4. NativeConnAdapter[RAW_STATE_SYMBOL] (native.ts:1162): added for interface completeness. The workflow rollback path only flows through ActorContextHandleAdapter, so this is dead code in practice. Fine as-is.

Test Coverage

• New test is a good regression guard for the flush behavior; see naming concern above.
• Removed StripStepHistoryErrorDriver test is a coherent removal - it tested the stepData.error fallback that was deliberately deleted.
• Missing: no test covers stateless actor plus workflow steps. Once the crash is fixed, a test like 'should not crash when actor has no state and step fails' would prevent regression.
• Loop expectation change (toBe(2) to toBe(3)) appears correct: crash at iteration 3, state now only persisted after successful Loop.continue at end of iteration 2, so iterations 3-5 need to re-execute on resume.

Summary

The core approach is sound. Please address the stateless actor crash (and consider the vars clone concern) before merging.

[Updated review - see below for latest]

PR Review: fix(workflow-engine): only commit step state after success

Note: This PR is still in Draft status with an empty description/checklist.

Overview

Two related correctness fixes in the workflow engine:

1. State rollback on step failure - Snapshots actor state and vars before a workflow step runs and restores them if the step throws, preventing failed steps from leaking mutations into persisted actor state.
2. Deferred error flush - Removes flushStorage() calls from individual error branches, hoists the shared entry.kind.data.error / entry.dirty assignments above the if/else chain, and relies on the outer execution loop to flush.

Positives

• RAW_STATE_SYMBOL is the right approach: bypassing the write-through proxy for the snapshot avoids cloning a proxied object. Consistent implementation on both NativeConnAdapter and ActorContextHandleAdapter.
• Moving entry.kind.data.error = String(error) and entry.dirty = true above the if/else chain eliminates three nearly-identical copy-paste blocks. Good deduplication.
• The rollback assignments this.#runCtx.state = stateSnapshot and this.#runCtx.vars = varsSnapshot land correctly: set state() calls #writeState({ scheduleSave: true }) and the pending save is overwritten with snapshot data before the next flush checkpoint.

Issues

Misleading test name (clear bug)

The new test is titled "should not commit step error data to entry on failure" but the assertions directly contradict the name:

// The error should be committed for inspection.
expect(entry.kind.data.error).toBe("Error: step failed");

A better name: "should commit error but not output to step entry on failure".

Crash safety regression from flushStorage() removal

Before this PR, each error branch flushed immediately, persisting metadata.attempts, metadata.status, and entry.kind.data.error before rethrowing. Now those are only flushed at the next outer-loop checkpoint (e.g., before yielding/sleeping).

The crash window is:

1. metadata.attempts++ and metadata.dirty = true set before the try block
2. Step fails - error branch runs, no flushStorage()
3. Process crashes before the outer loop flushes
4. On recovery: metadata.attempts is stale (under-counted), the step retries with wrong attempt count, and may silently exceed maxRetries

This window was effectively zero pre-PR. A comment on the error catch block documenting that the caller (outer loop) must flush before sleeping/terminating would make the invariant explicit.

Migration risk from stepData.error ?? metadata.error to metadata.error

The old fallback handled partial-write scenarios where the history entry was written but entry.kind.data.error was absent (crash mid-flush). Since both fields are now set before any flush, this fallback is structurally redundant for new executions. However, workflows that crashed mid-flush under the old code may have metadata.error set but entry.kind.data.error absent in storage. Replaying with new code produces undefined error messages on StepExhaustedError for those in-flight workflows. Low risk for non-production deployments, worth noting for production.

structuredClone edge case (minor)

structuredClone throws on non-serializable values (class instances, Map, Set, functions). If actor state or vars contain such values, the snapshot itself throws before #withActorAccess runs, leaving the step un-executed with a confusing error. Zod validation on user-defined schemas largely prevents this, but a one-line comment noting the assumption would help.

Test Coverage

• Removing StripStepHistoryErrorDriver is justified: the scenario it covered (metadata has error, history entry does not) is now structurally prevented.
• The loop test expectation change (iterationsExecuted[0]: 2 to 3) is correct: with state rollback, iteration 2's state commits durably after Loop.continue, so iteration 3 is the replay start on crash-recovery.
• Gap: No test covers the crash-recovery scenario for under-counted retries (process dies after step fails, before the outer flush). This is the main correctness risk introduced by the flushStorage() removal.

Summary

The RAW_STATE_SYMBOL snapshot/restore approach is clean and the deduplication is a clear improvement. Three things before merging:

1. Fix the test name (it says "should not commit" but asserts that it does).
2. Add a comment on the catch block documenting the caller-must-flush invariant, or add a test for crash-recovery with mis-counted retries.
3. Note the partial-write migration risk for any workflows in-flight under the old code.